@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1042/pipeline

@nodejs/tsc PTAL

any reason not to define these on the prototype, outside the constructor?

@devsnek no, that's actually a good point. It might not work in all cases but probable in almost all.

That will also reduce the performance overhead and it should be possible to do that pretty much everywhere.

Just a general question: if we actually want to make these properties private, until private class fields/methods land in ECMAScript, why can't we just use the WeakMap semantics, instead of symbols (which are not private, at least until private symbols is in ECMAScript/we decided to actually use V8 private symbols)?

On how the WeakMap semantics can be used for private states, see

@joyeecheung i believe we still want them to be inspectable for debugging purposes

@joyeecheung this would be slower and some times it's good to have the possibility to inspect them. Likely we have a couple of places in which it would indeed be fine to hide them though.

@devsnek Thanks, that sounds like a good justification for symbols to me.

this would be slower and some times it's good to have the possibility to inspect them. Likely we have a couple of places in which it would indeed be fine to hide them though.

@BridgeAR I am curious if using WeakMap is actually slower than symbol properties...do you have some numbers? (If not, I'll do some investigation myself)

@joyeecheung no, I would have to run some benchmarks on it as well.

I addressed a couple of comments. PTAL.

@Fishrock123 I'll wait with that until CITGM returns good results again (that should be soon).

@devsnek @joyeecheung @mcollina @addaleax PTAL

@nodejs/util @devsnek @joyeecheung @mcollina @addaleax PTAL

@nodejs/tsc PTAL

I'm ok with this in general but can do you a benchmark to see if this has any perf impact.

@addaleax But then util.inspect.defaultOptions.showHidden = true will still make them available during debugging?

@joyeecheung Yeah, that’s what Ruben said as well (https://github.com/nodejs/node/pull/23126#discussion_r221111038)… I still think it’s bad UX to require that (and even to require knowing about that – I assume that < 5 % of JS devs or so would be aware of util.inspect.defaultOptions).

@addaleax

I assume that < 5 % of JS devs or so would be aware of util.inspect.defaultOptions

But what would be the percentage of people who are actually able to interpret these hidden properties?

@joyeecheung I think that varies a lot between the different call sites (or what you want to call it) – that’s kind of my point. For example, the kState property of a Hash instance should be easy enough to interpret, and might be useful. On the other hand, the kNativeDecoder property of a StringDecoder property is only useful to core devs – but it would still be nice if it came by default with any bug report we get for string_decoder.

@addaleax hmm, but I don't really recall seeing a lot of bug reports with a inspection of any hidden property? Usually the best we can get are error stacks, and if it's hard to reproduce something locally, the code is probably running on some server where you don't even want to randomly log out a giant unknown object using util.inspect (which could potentially freeze your process).

@joyeecheung Yeah, I get that…

Maybe the bigger issue I’m having with this is that this PR reduces people’s ability to understand how things in core work, and our code is already pretty inaccessible to a lot of people. One of things I like so much about JS, and that helped me a lot when learning to code, is that you can see anything that’s happening under the hood; making properties non-enumerable always introduces a bit more magic into everything. (That’s also a major issue that I have with introducing private properties into JS, but that’s another story and not something I can do much about…)

I don't have a very strong opinion but I think we should in general keep our objects as much as possible inspectable. We use Symbols not to hide useless/un informative data but to prevent programmatic access to implementation details.

@addaleax I see, but for me the actual way to see everything under the hood, is to run a debugger and set breakpoints instead of looking at console.log outputs, guessing what the properties mean, and reading static code that you probably don't build locally and that may not even match your binary..it's fairly easy to debug into the JS code in a release, at least.

@joyeecheung Yeah, and for example, I’m largely avoiding debuggers for JS unless I have to use them – I’m glad they’re available, and sometimes they are extremely useful, but most of the time they seem to be getting in my way (I guess you could call that personal taste)?

@addaleax Maybe instead of trying to hold on to certain feature and hoping people to find out and use it to debug core, a better idea would be to actually write a debug guide, then we are free to make any adjustment to internals without worrying making core more inaccessible provided that we update the guide properly.

@joyeecheung I don’t think keeping properties (I wouldn’t call them features?) is conflicting with making adjustments to internals in any way, tbh. We started using Symbols for internals a while ago, and I haven’t seen any real-world code that would lock us in that uses these private Symbols?

Basically, I think what we have is effective enough, and making them non-enumerable won’t help significantly with the issue of internals usage…

AFAIK using symbols was a reaction to people using internals which should not have been used in userland. That's why we struggled removing the undocumented underscored properties. Symbols are more difficult to access but all of these details are actually implementation details which IMO bring very little benefit to anyone outside of core. People who actually know what to do with the symbols should also be able to actively opt into making them visible. Otherwise it's best to keep them hidden. I compare this to the debug module (or util.debuglog) which hides all information by default and just in debug mode actually shows further details.

And we actually had people trying to manipulate and use the symbol properties as well...

I normally don't use the debugger either and use console.log instead. But to do so, I add some logging inside of core which is not yet available. If I need the hidden information, I'll just write my log statement accordingly.

I would like to draw a real line between internal properties and properties which any user should know about. That's why I want the symbols being hidden (besides the fact that they clutter the output with information which is rarely useful to anyone outside of core).

@BridgeAR That sounds a lot like drawing a fixed line between core contributors and Node’s consumers, and I don’t think that’s a particularly good thing either…

@addaleax I don't see it like that. Any Node.js consumer can also become a contributor. All applications have implementation details which the potential contributor has to become familiar with when starting to work on that code and that's just the same here.

Refs: #21005

@addaleax is there any middle ground for this PR? Would you be OK with this if I revert the crypto and trace_events changes?

@nodejs/tsc PTAL. This needs some reviews.

I'm really not convinced this will help us or our users. My opinion is still that the more information is output by default, the better.
I doubt that many people will actually start using properties hidden behind symbols because they saw them in logs.

I agree with @targos... I'd prefer what exists to actually show up in inspects.

This was discussed in the TSC meeting today. Understanding is it was added to agenda for awareness, discussion was continuing in github and to ask more TSC members to comment. Will remove tsc-agenda. If necessary please re-add along with adding the specific question for the TSC.

Maybe a middle ground would be to provide an option to hide the non-enumerable symbol properties, and then we can go ahead bikeshedding about whether this option should be on or off by default? I think the disagreement in this thread is:

1. Some people feel that having these properties displayed by default is useful
2. Some people feel that they are not useful (or they make it less useful because of the length of the output)

But we can at least have options if we make an option.

Also it might be nice to have an option to specify the level of verbosity in util.inspect, similar to what you usually have in loggers - depth is not always a very good measure, and showHidden is too board. But I am not certain about how the feature can be designed (public, protected, private?). At the very least I think it would be nice to be able to hide all the underscore properties when an option is specified (try logging out res/req in an express app and you'll understand). It helps users to understand what is more likely to be public APIs by simply navigating through console.log instead of reading the doc (of course they can confirm in the docs but learning in the repl is a nice thing to do), but having too much information in the output (like underscore properties with deeply nested structures) makes it hard to do that.